Repository navigation
fix(gates): irrecoverable reads each global value as one shell word (GH #375) - #383
Merged
Merged
Conversation
…d (GH #375) shlex splits one shell word at a character outside its wordchars, so `git -C $PWD reset --hard` reached the git walk as -C, $, PWD and PWD was read as the subcommand. _tokens now marks a token cut from the one before it (_Glued), and the git block also checks a window whose global flags are joined back into shell words. The token reading stays, so a verdict can only tighten.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #375. Follow-up for the sibling walks: #382.
Root cause
_tokensruns shlex withpunctuation_charsand nowhitespace_split. A character outside shlex's wordchars ends the token, so one shell word becomes several tokens:$PWDis$,PWD;a:b,a@b.cand a Thai path split the same way. The git global walk skips one token per value, so ingit -C $PWD reset --hardit tookPWDfor the subcommand and no git rule sawreset. The same split hit-C$R,--git-dir=$G,-c k=$X,-c user.email=a@b.cand-C ~/<Thai path>. These all exit 0 on develop.Fix
_tokensreturns a token that shlex cut from the one before it, with no blank between them, as_Glued, an equalstrsubclass. Every other check reads the same strings as before. Glue is read from the lexer: a token cut at a non-word character pushes that character back, and a one-character token ($) ends without reading on._git_wordsre-reads the leading global flags with each glued run joined into one shell word, using the same walk (a value-taking global takes the next word). When anything was joined,["git"] + wordsis checked as a window of its own. The token reading stays, so a deny from either reading wins and no develop deny is lost. The subcommand word's own glue does not count, so the new window never re-appends itself._sets_hooks_pathgets the joined value too, so-c core.hooksPath=$Hnow denies.find -execreach the same git block.var-verb (#376)
Its only deny hit in the replay,
R=...; git -C $R checkout -- <file>, is now denied bygit-checkout-discardin the main pass, before any view runs. On this PR's replay,would_deny:var-verbgoes from 1 to 0, and its 2would_askrows are unchanged (one is a realGH=gh; $GH pr mergeread through the view). So var-verb is redundant for that hit. It stays inSHADOW_RULES; promoting it is a separate call. The 336 fixture row for that command is nowDENY.Tests (red first)
tests/hooks/fixtures/irrecoverable-375-cases.txt, replayed bytest-gates.sh: 41 DENY rows and 17 ALLOW controls. 40 DENY rows were red on develop: every destructive rule after the variable (reset --hard, push --force / -f / +ref, clean -f, checkout --, restore, switch -f, branch -D, stash drop/clear, commit --amend / -n, add -A), every value global, glued and spaced forms, other split characters, and the sudo / env / xargs / find routes. The 41st,git -C $reset --hard, already denies on develop and stays to pin the token reading (not destructive: withresetunset git gets-C --hard). Controls:git -C $REPO status,git -C "$d" log,git -C $x diff,git -c user.email=a@b.c commit -m x,git -C ~/<Thai> status,git -C a:b diff,git -C $ status, afor d in */loop and others. An ALLOW control also fails if it journals a row.-C $Rglobals before reset --hard and before status, 2,000git -C $R status;statements then a reset, a 20 KB glued$a$a...value, and glue only inside the subcommand word (must not loop).~/my\ $R), glue ignoring the blank before, glue ignoring the blank after, join every token, value global taking no word, no glued window, subcommand glue counted (an endless window loop, killed by thestatus$Xtiming row), and the window replacing the token reading (killed bygit -C $reset --hard).test-gates.sh1310/1310,test-gate-journal.sh30/30 after the rebase.harness-audit0 CRIT. Gauntlet passes withGAUNTLET_JOBS=2.Differential (frozen develop vs this branch, one process, every payload as main and as agent)
Compared (exit code, stdout, stderr, journal rows). No exit other than 0/2 except 2 payloads in the test fixtures (a list-valued command, the same traceback in both).
No verdict got looser, no ask changed, no message changed on a command that was already denied, and no diff was journal-only.
Replay, one by one (3 distinct commands, now denied):
git -C $R checkout -- <file>discards a working-tree file (the var-verb hit). Destructive.git -C $PCR reset -q --hardandgit -C $PCR add .. Destructive.git -C $PCR add .(git-add-all). This is the 1 false deny by the "runs a destructive git subcommand" bar: staging everything is policy, not data loss. The gate already deniesgit add .andgit -C "$PCR" add .in every other spelling.Fuzz, real shells (each distinct diffed command run in 5 shells with a stub, argv checked against develop): 1,626 of 1,852 ran a destructive git argv in at least one shell. The other 226, all the safe direction:
builtin git ...: develop already treatsbuiltinas a wrapper, but the shell refuses it, so git never runs.git -C $R checkout $BR: the unquoted value now reads like the quoted one, and develop deniesgit -C "$R" checkout $BR(bash and dash split BR="HEAD f" into a tree-ish and a path). The same holds forgit -C $R branch -D feat, which_BRANCH_D_PLAIN_REonly allows for a plain-Cpath.$(..)), or a noise character glued into the subcommand or a flag (clean+ -fd,--h+ard), which develop reads as tokens too. One of these really is destructive (-C '' $X reset --hard); the oracle missed it because of a develop gap filed in gate: irrecoverable wrapper and assignment walks read a split variable as the command #382.2>/d!ev/null), so git never runs.Timing (whole gate process,
python33.14; old vs new): 4,000-C $R+ status 0.06 s / 0.07 s; 140 KB of$awords 0.18 s / 0.24 s; 140 KB glued$avalue 0.23 s / 0.29 s; 132 KB of-c k=$a:b@c0.18 s / 0.23 s. Replay worst case 0.07 s, fuzz 0.06 s. The differential and the mutants ran under/usr/bin/python33.9.6, the test suites under 3.14._pushback_charsandinstream.tell()are shlex internals: a missing_pushback_charsfalls back to(), which only loses glue and never loses a develop deny.Residue (#382)
The same one-token reading in other walks still allows
sudo -u $U git reset --hard,env -u $X git ...,timeout $T git ...andX=$Y rm -rf build/X=$Y git reset --hard. Separately,git -C "" reset --hardis allowed because_split_ops("")drops the empty token. Those fixes deny non-git commands too, so they need their own differential. A literal$value (git -C $ reset) andgit $X reset --hardread as before.No manifest bump.